-
Notifications
You must be signed in to change notification settings - Fork 214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(fix) O3-4298: Capitalize language names in the change language
modal
#1254
(fix) O3-4298: Capitalize language names in the change language
modal
#1254
Conversation
cdfc659
to
02008a5
Compare
@Muppasanipraneeth British English should have 'E', not 'e'. |
fixed British english to British English Screen.Recording.2025-01-04.at.11.35.20.AM.mov |
@@ -51,6 +51,9 @@ export default function ChangeLanguageModal({ close }: ChangeLanguageModalProps) | |||
), | |||
[allowedLocales], | |||
); | |||
const Capitalize = (str: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we must capitalize these language names, I think it'd be preferable to use the capitalize
function from lodash-es
here over something custom.
import { capitalize } from 'lodash-es';
@@ -69,7 +72,7 @@ export default function ChangeLanguageModal({ close }: ChangeLanguageModalProps) | |||
key={`locale-option-${locale}-${i}`} | |||
id={`locale-option-${locale}-${i}`} | |||
name={locale} | |||
labelText={languageNames[locale]} | |||
labelText={Capitalize(languageNames[locale])} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
labelText={Capitalize(languageNames[locale])} | |
labelText={capitalize(languageNames[locale])} |
@@ -34,10 +34,10 @@ describe(`Change Language Modal`, () => { | |||
it('should correctly displays all allowed locales', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking, none of the strings in this file need to change to uppercase because the i
flag in the regex will make the match case insensitive anyway. So we can safely undo the changes here.
Also, I don;'t
I don't think this is strictly necessary in this context. While technically "English" is a proper noun and should be capitalized in both British and American English, what matters most here is:
The |
My only concern is how would this change affect non-english languages, where capitalization might not even be a concept? |
change language
modal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the right casing to me, thanks @denniskigen !
bf601e2
to
fc5529f
Compare
change language
modalchange language
modal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @Muppasanipraneeth!
Thanks @denniskigen for Reviewing😀 |
…al (openmrs#1254) * fix : changed the language modal * O3-4298 change the language modal * modified the test * rather than using capitalze from lodash-es done with function * used capitalize from lodash-es * updated the test * Fixup --------- Co-authored-by: Dennis Kigen <[email protected]>
Requirements
feat
,fix
, orchore
, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
The "Change language" modal lists some language names in lowercase (e.g., "français," "español"), which is inconsistent with the proper capitalization of language names like "English" or "British English."
Screenshots
Before
Screen.Recording.2025-01-03.at.11.14.05.AM.mov
After
Screen.Recording.2025-01-03.at.11.43.21.AM.mov
Related Issue
Other